Repository navigation
test: fix Bun-canary failures (quoted CSS url(), cache marker polling race) - #375
Merged
Merged
Conversation
Bun canary (oven-sh/bun#47025) always quotes the copied asset path it splices into url(), so the unquoted-only regex matched nothing and the canonical-name assertion saw undefined.
…aits The cross-process marker tests polled for the marker for ~0.5s, racing the durable value write and marker removal on slow CI disks. Wait on fn being entered (the marker is written first) and on whenIdle() (settles after marker removal) instead, then assert the marker state directly.
Contributor
Mochi review reportTry this PRExpand instructionsgh run download -R khromov/mochi 40983939550 -n mochi-framework-pr -D /tmp/mochi-pr && bun i /tmp/mochi-pr/mochi-framework-pr.tgzDependency reportExpand reportSecurity & licenses
|
| Category | main | PR | Δ |
|---|---|---|---|
src/**/*.test.ts |
25756 | 25783 | +27 |
| Total | 54987 | 55014 | +27 |
Unchanged: src/Mochi.ts (2116), src/{events,log,logger}.ts (469), src/extensions.ts (318), src/image/** (1797), src/middleware/** (72), src/{types.ts,*.d.ts} (977), src/web-components/** (756), src/debug-bar/** (3436), src/templates/** (1720), Other (17570).
packages/docs
| Category | main | PR | Δ |
|---|---|---|---|
| Total | 6710 | 6710 | 0 |
Unchanged: Docs (6710).
packages/site
| Category | main | PR | Δ |
|---|---|---|---|
| Total | 24564 | 24564 | 0 |
Unchanged: src/demos/** (14189), src/components/** (3032), src/lib/** (2688), src/stores/** (26), Other (4629).
packages/demos
| Category | main | PR | Δ |
|---|---|---|---|
| Total | 3064 | 3064 | 0 |
Unchanged: src/hn/** (1072), Other (1992).
packages/minimal
| Category | main | PR | Δ |
|---|---|---|---|
| Total | 550 | 550 | 0 |
Unchanged: Other (550).
packages/minimal-rsvelte
| Category | main | PR | Δ |
|---|---|---|---|
| Total | 548 | 548 | 0 |
Unchanged: Other (548).
packages/support
| Category | main | PR | Δ |
|---|---|---|---|
| Total | 3301 | 3301 | 0 |
Unchanged: src/**/*.test.ts (630), src/admin/** (671), src/components/** (84), src/*.server.ts (379), src/{index,routes}.ts (237), Other (1300).
packages/shared
| Category | main | PR | Δ |
|---|---|---|---|
| Total | 44 | 44 | 0 |
Unchanged: Other (44).
packages/mochi-rsvelte
| Category | main | PR | Δ |
|---|---|---|---|
| Total | 227 | 227 | 0 |
Unchanged: Other (227).
packages/mochi-svelte-shaker
| Category | main | PR | Δ |
|---|---|---|---|
| Total | 184 | 184 | 0 |
Unchanged: Other (184).
packages/cli
| Category | main | PR | Δ |
|---|---|---|---|
| Total | 1066 | 1066 | 0 |
Unchanged: src/**/*.test.ts (440), src/cli* (165), src/{create,templates,utils}.ts (277), Other (184).
packages/video-animations
| Category | main | PR | Δ |
|---|---|---|---|
| Total | 646 | 646 | 0 |
Unchanged: src/{frame,anim,theme}.ts (389), src/{render,render-worker,generate}.ts (182), Other (75).
packages/msgpackr-extract-stub
| Category | main | PR | Δ |
|---|---|---|---|
| Total | 29 | 29 | 0 |
Unchanged: *.test.ts (22), Other (7).
The font/asset tests pulled url() and format() values out of Bun's printed CSS with regexes, so they depended on Bun's quoting. Read them via parseCss, as production does, which decodes quoting.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the two Bun-canary test failures in
test.yml. Both are test-only changes; no framework code changes.1.
cssFontImports.test.ts— consistent canary failureRoot cause: an intentional Bun output change, not a Bun bug or a mochi bug. oven-sh/bun#47025 ("css: consolidate the open CSS parser fixes", commit
794200868, between 1.4.438afee6c8and canaryda867c60b) now prints:url()is always quoted, so a file name with a space or a parenthesis cannot end theurl()" (#41557)The test extracted the asset name with
/url\(\.\/([^)"']+\.png)\)/, which only matches the unquoted form. On canary, every sheet returnedundefined, sonameswas{undefined}. Its size of 1 passed, andtoMatchthrewReceived value must be a string: undefined.Standalone repro (no mochi):
Bun.build({ entrypoints: ['a.css'], minify: true })on.a{background:url("./shared.png")}with a 200 KB png:Production code is unaffected, because
cssFontAssets.tsreadsurl()andformat()through the css-tree AST (parseCssincssAst.ts), which decodes quoting.Fix (after review): the tests now read bundled CSS the same way.
cssFontImports.test.tsgains three test-local helpers:parsed()wrapsparseCssand throws a clear error on anullresult.urlValues()returns the decodedurl()values.relativePng()returns the first non-data:.pngurl containing a given stem.Converted from regex extraction to the AST, each kept at least as strict:
:98checks the served woff2 URL and itsformat()on the samesrcentry, where the old regex only required them to be adjacent text.:105checks that no font source hasformatwoff, guarded against an empty source list.:302checks that no relative (non-/_mochi/fonts/).woff2url is left.:388,:401,:407,:460,:487read the relative image urls.:550is the canonical-name test. Each sheet must contribute exactly one./-relative.pngurl, otherwise it addsundefinedto the set. There must be exactly one name, and it must match^shared-[a-z0-9]{8}\.png$.Left as text checks on purpose:
toContain/not.toContainpresence checks on URLs anddata:payloads (:108,:113,:192,:202,:233,:299-301,:312-313,:323-324,:400,:459,:543). They don't extract a value, they don't depend on quoting, and most match mochi's own substituted text.cssImports.test.ts:57-59asserts the raw minified text.cssImports.test.ts:123-124asserts the serialized quoting offormat('woff2-variations')itself. The AST'sformatlowercases both the string and the identifier form, so it cannot tell them apart.cssFontAssets.test.tschecks mochi's ownsubstituteFontUrls/classifyFontAssetsoutput on hand-written input, with no Bun involved.No production change:
parseCssalready exposes everything the tests needed.Mutation checks on the converted file, with production code broken temporarily and then restored:
canonical = assetPath)drops the legacy woff source entirelyfailssubstituteFontUrlswrite a wrong URLrewrites the large woff2…fails2.
cache-storage-file.test.ts— probable flakeI could not reproduce it. I ran it 0/320 failures: 100 runs each on stable and canary at idle, and 60 runs each with 24 busy loops on 12 cores plus sync
ddI/O. Reading the test does show a real timing assumption, though.waitForMarkerpolled for at most 250 × (getItem + 2 ms), which is about 530 ms of wall time. Afterrelease(), A must do a durablesetItem(fsync + rename) and then remove the marker. Locally that takes about 2 ms. This file's ownLEASEcomment records CI runs of 400–1300 ms per test.Injecting an 800 ms delay into A's
setItemreproduces the exact CI error and stack:marker for "k" never became absentfromwaitForMarker, called from the line afterrelease().The fix replaces the fixed-budget polling with deterministic synchronization:
fnis entered (run()awaitswriteMarkerbefore callingfn), then assert that the marker exists.await cacheA.whenIdle()(it settles only after the run'sfinally { await removeMarker }), orawait secondin the superseded-run test. Then assert once that the marker isnull.These checks are at least as strict as before: the marker must be gone the moment the run settles, with no grace window. With the 800 ms injection, the new test passes. With
removeMarkermutated out, the cross-process tests still fail.waitForMarkeris deleted because nothing uses it now.Verification
bun run checks(stable 1.4.4): pass.mochi-frameworkpassed 204/204 test files.bun run build(stable 1.4.4): pass.cssFontImports.test.ts+cssImports.test.ts: 31/31 on stable and on canary.cssFontImports.test.ts22/22 on stable 1.4.4 and on canary 1.4.4-canary.1 (da867c60b).bun run checks(stable): pass.cache-storage-file.test.ts: 37/37 on stable and on canary. The cross-process block also passed 40/40 on each version under CPU load.No upstream issue is needed: the Bun change is intentional and valid CSS.